cranelift: Allow `call` and `call_indirect` in runtests #4667

afonso360 · 2022-08-09T20:35:46Z

👋 Hey,

This is an in progress PR just to share my current ideas for where to take the runtest suite.

It is based on top of #4453 and rewrites a lot of the code, but I felt it would be easier to review as a separate PR.

This PR does a number of things.

Changes the order that we run tests from per func, per target to per target, per func.
Compiles the entire file once per target and then calls the trampolines for each run invocation
Renames functions in runtest files, since they must now be unique.
Reuses trampolines based on signature (something we lost in cranelift: Use cranelift-jit in runtests #4453)
Internally auto renames functions to ExtName::User so that they are callable in JIT.
Adds tests with call and call_indirect !

TODO (for a future PR):

Register all functions at once in test interpreter so that we can also enable call tests there
Add a list of allowed CallConv's per target, so that we can test calling between different CallConv's
Generate call's in the fuzzer

github-actions · 2022-08-09T23:39:08Z

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "cranelift", "fuzzing"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

cranelift/filetests/filetests/runtests/call.clif

afonso360 · 2022-08-17T10:03:21Z

@jameysharp @cfallin I think this is ready, would you be able to review it?

jameysharp

I haven't had opportunity to give this a thorough review yet, but I've skimmed it, and at a high level I think it's a good idea. Not only does this PR allow us to test cases we can't test today, but it sounds like it can run the tests faster (by reusing compilation results and by short-circuiting unnecessary work sooner).

So far my only complaint is that the 16-character limit on function names forces changing a lot of tests, when I would think it'd be better to just increase the limit on name length. At a quick glance I didn't see where that length limit comes from. Does making that limit bigger have any effect on other uses of Cranelift, or is it just a limitation of the filetest infrastructure?

jameysharp · 2022-08-17T16:47:37Z

cranelift/filetests/filetests/runtests/atomic-rmw-subword-big.clif

+; run: %rmw_add_big_i16(0x12345678, 0, 0x1111) == 0x23455678
+; run: %rmw_add_big_i16(0x12345678, 0, 0xffff) == 0x12335678
+; run: %rmw_add_big_i16(0x12345678, 2, 0x1111) == 0x12346789
+; run: %rmw_add_big_i16(0x12345678, 2, 0xffff) == 0x12345677


Tell me if I have this right: these tests only worked before because the actual function name was ignored, so it didn't matter that the run comments said little when the function declaration said big.

Yes, test run (used to) ignore the actual name of the function.

We have had similar issues when we started adding test interpret to the runtests, since it calls functions by their ExternalName, however the interpreter also isn't used in this file.

cfallin · 2022-08-17T17:42:44Z

So far my only complaint is that the 16-character limit on function names forces changing a lot of tests, when I would think it'd be better to just increase the limit on name length

Historically at least, the limit was to keep the size of ExternalName reasonable; these are embedded and carried in the ir::Function and in "production" use, are two u32s only (the u0:0 format) because the embedder is supposed to own names and Cranelift handles indices only. I wasn't around for that design decision and I'm not sure if it would make sense to support a boxed variant for the "one-off test function with an owned name" case; that would be the way to go, IMHO, rather than bumping the limit and inflating the fixed-size field.

jameysharp · 2022-08-17T17:52:44Z

Maybe the test-runner should keep a mapping between heap-allocated names and the "pair of u32" representation, just like an embedder would?

afonso360 · 2022-08-17T17:55:49Z

Maybe the test-runner should keep a mapping between heap-allocated names and the "pair of u32" representation, just like an embedder would?

I think this would be a good idea, if we can remove TestCase from ExternalName I think the struct size goes down, so it could potentially reduce memory usage and speedup some stuff!

Edit: Had a look around the code, and we probably also need to change the parser and a few other things for this.
I think it would be best if it was a separate PR, I can put this on hold while I try to figure it out.

jameysharp · 2022-08-18T16:05:28Z

I think this would be a good idea, if we can remove TestCase from ExternalName I think the struct size goes down, so it could potentially reduce memory usage and speedup some stuff!

I can confirm it's currently 20 bytes with TestCase, and only 8 bytes without, so that sounds good!

I've just done an experiment, reducing the length limit of test-case names to 6 bytes, so it fits inside an 8-byte ExternalName without having to make all the changes needed to actually remove TestCase. I'm running valgrind --tool=dhat target/release/wasmtime compile on the pulldown-cmark benchmark from Sightglass, with the wasmtime CLI built with --no-default-features to disable parallel compilation and other sources of nondeterminism.

With that experimental setup, the smaller ExternalName makes a positive difference, but it's... small. Total bytes allocated drops by 40,504 bytes out of about 171MiB. Maximum heap size drops by 1,096 bytes out of about 12MiB. And the program executes about 1 million fewer instructions, out of close to 2 billion.

So I think this is totally worth doing, but... don't get your hopes up for much improvement in memory usage or compile speed. 😁 I was really curious which way this would go, and it was fun doing the quick experiment.

afonso360 · 2022-08-23T18:11:03Z

With that experimental setup, the smaller ExternalName makes a positive difference, but it's... small. Total bytes allocated drops by 40,504 bytes out of about 171MiB. Maximum heap size drops by 1,096 bytes out of about 12MiB. And the program executes about 1 million fewer instructions, out of close to 2 billion.

That's... not as much as I hoped for 😄. Thanks for checking anyway!

I started working on this refactor a couple days ago (in progress branch), but it's going to touch a lot of stuff and I don't think its going to be ready anytime soon.

@jameysharp Would it be ok to merge this with the test renames? I'd like to build on this to fix #4758 but its going to take a while if I have to do the refactor first and I'd prefer to use that time to fix the other fuzz issues.

jameysharp · 2022-08-23T19:53:57Z

I was hesitant, but actually yeah, renaming the test functions to fit within the limit is fine. Let's just plan that later we'll do a partial revert to get the more descriptive test names back.

One alternative that comes to mind is to leave the TestCase option in ExternalName, but make it a Box<[u8]> instead. Then ExternalName has the same size as it does now but hopefully it's easier to switch things over.

I'm happy enough either way though.

That said, I haven't had a chance to review this more carefully yet, so I'm not ready to merge it. Maybe @cfallin can get it into his review queue but he's got a lot on his plate. So maybe we can chat in #4758 about whether there are easier ways to fix the immediate bug there.

afonso360 · 2022-08-24T09:13:53Z

I've filed #4766 to track this refactor.

One alternative that comes to mind is to leave the TestCase option in ExternalName, but make it a Box<[u8]> instead. Then ExternalName has the same size as it does now but hopefully it's easier to switch things over.

Thanks for picking this up! I didn't even consider that as an option, but its a great middle step and solves the immediate issue.

That said, I haven't had a chance to review this more carefully yet, so I'm not ready to merge it. Maybe @cfallin can get it into his review queue but he's got a lot on his plate. So maybe we can chat in #4758 about whether there are easier ways to fix the immediate bug there.

👍

Changes the ordering of runtests to run per target and then per function. This change doesn't do a lot by itself, but helps future refactorings of runtests.

With the upcoming changes to the runtest infrastructure we require unique ExtNames for all tests. Note that for test names we have a 16 character limit on test names, and must be unique within those 16 characters.

TestFileCompiler allows us to compile the entire file once, and then call the trampolines for each test. The previous code was compiling the function for each invocation of a test.

The JIT internally only deals with User functions, and cannot link test name funcs. This also caches trampolines by signature.

It looks like we forgot to delete it when it was moved to `i128-bricmp-overflow`, and since it didn't have a run invocation it was never compiled. However, s390x does not support this, and panics when lowering.

With the latest main changes we now support *unlimited* length test names. This commit reverts: 5227467 7989edc 25c8a83 792e8cf

These weren't auto reverted by the previous revert.

cfallin

This all looks correct to me -- thanks for this refactor, as I think it'll be a really useful feature!

The function-renaming business is a little subtle but I don't have any easy suggestions for cleaning it up. It does seem like the "right" answer would be for cranelift-jit to keep a symbol table and support resolving references to testcase names, but that's probably just as complex as this change; and if it does eventually grow that for other reasons then we can remove the renaming layer here.

cranelift/filetests/filetests/runtests/i128-bricmp.clif

cranelift/filetests/src/function_runner.rs

jameysharp · 2022-08-26T21:53:31Z

Well, darn. This seems to have broken cranelift-fuzzgen, and I don't understand why. But 20 out of 50 cases I tested from my existing corpus crash now, and it looks like they're all with one of these two messages:

thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Undeclared function u0:0 is referenced by u0:1!', fuzz/fuzz_targets/cranelift-fuzzgen.rs:73:53
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Duplicate function with name u0:1 found!', fuzz/fuzz_targets/cranelift-fuzzgen.rs:76:10

jameysharp · 2022-08-26T22:56:49Z

Okay, I'm reasonably convinced that "Undeclared function u0:0 is referenced by u0:1" has the same root cause as #4757 and #4758, although it's a different symptom. Previously these undefined functions caused failures if the interpreter tried to actually call them, or if the JIT tried to compile a call instruction referencing them. Now the new code also fails if there's a function signature declared, even if that function is never called.

I've opened PR #4795 for the other error. It's a small fix.

This fixes bytecodealliance#4757, fixes bytecodealliance#4758, and fixes new fuzzbugs that are probably coming after we merged bytecodealliance#4667.

This fixes #4757, fixes #4758, and fixes new fuzzbugs that are probably coming after we merged #4667.

afonso360 changed the title ~~Runtest multifunc~~ cranelift: Link entire runtest files Aug 9, 2022

afonso360 changed the title ~~cranelift: Link entire runtest files~~ cranelift: Allow call and call_indirect in runtests Aug 9, 2022

afonso360 mentioned this pull request Aug 9, 2022

cranelift: Use cranelift-jit in runtests #4453

Merged

github-actions bot added cranelift Issues related to the Cranelift code generator fuzzing Issues related to our fuzzing infrastructure labels Aug 9, 2022

afonso360 mentioned this pull request Aug 16, 2022

Initial forward-edge CFI implementation #3693

Merged

afonso360 force-pushed the runtest-multifunc branch from 7c02c28 to 1d90fef Compare August 16, 2022 23:08

afonso360 commented Aug 17, 2022

View reviewed changes

cranelift/filetests/filetests/runtests/call.clif Show resolved Hide resolved

afonso360 marked this pull request as ready for review August 17, 2022 09:58

jameysharp reviewed Aug 17, 2022

View reviewed changes

afonso360 mentioned this pull request Aug 23, 2022

cranelift-fuzzgen fuzzbug: "interpreter failed: Error(StepError(UnknownFunction(fn0)))" #4758

Closed

jameysharp mentioned this pull request Aug 24, 2022

Don't limit ExternalName::TestName length #4764

Merged

afonso360 mentioned this pull request Aug 24, 2022

cranelift: Remove TestName as an option for Function names #4766

Open

afonso360 added 8 commits August 24, 2022 11:36

cranelift: Change test runner order

eb9da04

Changes the ordering of runtests to run per target and then per function. This change doesn't do a lot by itself, but helps future refactorings of runtests.

cranelift: Rename SingleFunctionCompiler to TestCaseCompiler

26f320b

cranelift: Skip runtests per target instead of per run

e81127b

cranelift: Deduplicate test names

46e2c0d

With the upcoming changes to the runtest infrastructure we require unique ExtNames for all tests. Note that for test names we have a 16 character limit on test names, and must be unique within those 16 characters.

cranelift: Add TestFileCompiler to runtests

75518d5

TestFileCompiler allows us to compile the entire file once, and then call the trampolines for each test. The previous code was compiling the function for each invocation of a test.

cranelift: Deduplicate ExtName for avg_round tests

8fc50c0

cranelift: Rename functions as they are defined.

62bd895

The JIT internally only deals with User functions, and cannot link test name funcs. This also caches trampolines by signature.

cranelift: Preserve original name when reporting errors.

2654e31

afonso360 added 10 commits August 24, 2022 11:36

cranelift: Rename aarch64 test functions

88771d7

cranelift: Add call and call_indirect tests!

98c1452

cranelift: Add pauth runtests for aarch64

fb04e83

cranelift: Rename duplicate s390x tests

644de9f

cranelift: Delete i128_bricmp_of function from i128-bricmp

3c18bf8

It looks like we forgot to delete it when it was moved to `i128-bricmp-overflow`, and since it didn't have a run invocation it was never compiled. However, s390x does not support this, and panics when lowering.

cranelift: Add colocated call tests

4562dd4

cranelift: Rename *more* s390x tests

314a058

cranelift: Add pauth + sign_return_address call tests

8fb1ca3

cranelift: Undeduplicate test names

1364ad2

With the latest main changes we now support *unlimited* length test names. This commit reverts: 5227467 7989edc 25c8a83 792e8cf

cranelift: Add LibCall tests

1eb3ac8

afonso360 force-pushed the runtest-multifunc branch from 073818f to 1eb3ac8 Compare August 24, 2022 10:54

afonso360 added 3 commits August 24, 2022 11:56

cranelift: Revert more test names

628d00c

These weren't auto reverted by the previous revert.

cranelift: Disable libcall tests for aarch64

ceccde2

cranelift: Runtest fibonacci tests

ad7f62d

afonso360 force-pushed the runtest-multifunc branch from c84a961 to ad7f62d Compare August 25, 2022 14:25

cfallin approved these changes Aug 25, 2022

View reviewed changes

cranelift/filetests/filetests/runtests/i128-bricmp.clif Show resolved Hide resolved

cranelift/filetests/src/function_runner.rs Outdated Show resolved Hide resolved

cranelift: Misc cleanup

dcd55cc

jameysharp merged commit 7a9078d into bytecodealliance:main Aug 26, 2022

jameysharp mentioned this pull request Aug 26, 2022

cranelift-fuzzgen fuzzbug: "Floating-point-exception in cranelift_filetests::function_runner::CompiledFunction::call::h6386b90d4c398abf" #4760

Closed

jameysharp mentioned this pull request Aug 27, 2022

Disable funcref generation for fuzz tests with inputs #4797

Merged

This was referenced Aug 27, 2022

cranelift: Register all functions in test file for interpreter #4800

Merged

cranelift: Test calling across different calling conventions #4801

Merged

jameysharp added a commit that referenced this pull request Aug 29, 2022

Disable funcref generation for fuzz tests with inputs (#4797)

4882347

This fixes #4757, fixes #4758, and fixes new fuzzbugs that are probably coming after we merged #4667.

afonso360 mentioned this pull request Feb 21, 2023

cranelift-interpreter: Implement the AtomicRMW instruction #5817

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cranelift: Allow `call` and `call_indirect` in runtests #4667

cranelift: Allow `call` and `call_indirect` in runtests #4667

afonso360 commented Aug 9, 2022 •

edited

Loading

github-actions bot commented Aug 9, 2022

afonso360 commented Aug 17, 2022 •

edited

Loading

jameysharp left a comment

jameysharp Aug 17, 2022

afonso360 Aug 17, 2022

cfallin commented Aug 17, 2022

jameysharp commented Aug 17, 2022

afonso360 commented Aug 17, 2022 •

edited

Loading

jameysharp commented Aug 18, 2022

afonso360 commented Aug 23, 2022

jameysharp commented Aug 23, 2022

afonso360 commented Aug 24, 2022

cfallin left a comment

jameysharp commented Aug 26, 2022

jameysharp commented Aug 26, 2022

cranelift: Allow call and call_indirect in runtests #4667

cranelift: Allow call and call_indirect in runtests #4667

Conversation

afonso360 commented Aug 9, 2022 • edited Loading

github-actions bot commented Aug 9, 2022

Subscribe to Label Action

afonso360 commented Aug 17, 2022 • edited Loading

jameysharp left a comment

Choose a reason for hiding this comment

jameysharp Aug 17, 2022

Choose a reason for hiding this comment

afonso360 Aug 17, 2022

Choose a reason for hiding this comment

cfallin commented Aug 17, 2022

jameysharp commented Aug 17, 2022

afonso360 commented Aug 17, 2022 • edited Loading

jameysharp commented Aug 18, 2022

afonso360 commented Aug 23, 2022

jameysharp commented Aug 23, 2022

afonso360 commented Aug 24, 2022

cfallin left a comment

Choose a reason for hiding this comment

jameysharp commented Aug 26, 2022

jameysharp commented Aug 26, 2022

cranelift: Allow `call` and `call_indirect` in runtests #4667

cranelift: Allow `call` and `call_indirect` in runtests #4667

afonso360 commented Aug 9, 2022 •

edited

Loading

afonso360 commented Aug 17, 2022 •

edited

Loading

afonso360 commented Aug 17, 2022 •

edited

Loading